Reduce size of Image and make it consistent no matter the featureset#11655
Reduce size of Image and make it consistent no matter the featureset#11655eira-fransham wants to merge 3 commits into
Image and make it consistent no matter the featureset#11655Conversation
Size on different platforms is not unified, so for example Wasm and native compilation may still result in different sizes of `Image`
… to allow use of `if let` guards
eira-fransham
force-pushed
the
reduce-size-of-image
branch
from
d00c721 to
55fea54
Compare
tronical
left a comment
tronical
left a comment
There was a problem hiding this comment.
I think it's a good idea to put the wgpu textures behind a VRc - generally speaking.
| license = "GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0" | ||
| repository = "https://github.com/slint-ui/slint" | ||
| rust-version = "1.92" | ||
| rust-version = "1.95" |
There was a problem hiding this comment.
I suggest to make this change without bumping the MSRV. If we decide to bump it, it's an effort that also requires changing documentation and it's something we do usually very consciously.
There was a problem hiding this comment.
Yeah, I only changed it because I wrote the PR to use if let guards and I thought this would be a quick way to test this on CI since I was mostly busy on my drag-and-drop PR. It ended up breaking CI even more though
|
|
||
| /// An embedded image, storing a cache key and the inline image data. | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| #[repr(C)] |
There was a problem hiding this comment.
Does this struct need to be repr(C) given that it's now behind the VRc?
There was a problem hiding this comment.
Ooh good point, yeah I don't think it does since we don't generate bindings to it for C++ at all
| #[expect(unused)] | ||
| const _: () = { | ||
| #[repr(u8)] | ||
| enum ImageInnerAllFeaturesDisabled { |
There was a problem hiding this comment.
I'm unsure if this is going to buy us much, TBH. We had issues with cfgs before here, but they were also related to propagation into cbindgen. So suppose the assertions end up failing here, we'll end up making a change here and that's it - could still introduce a missing #define on the C++ side...
There was a problem hiding this comment.
I could change it to cfg(all(test, not(target_family = "wasm"))), that means it'll be compiled on CI as a regression test. Also, I could make it explicitly check that the size is the same as:
struct Foo {
a: u8,
b: usize,
c: usize,
}instead of checking against a facsimile of the enum without features enabled. That should work as a cross-platform regression test at least.
| #[cfg_attr(not(feature = "ffi"), i_slint_core_macros::remove_extern)] | ||
| #[vtable::vtable] | ||
| #[repr(C)] | ||
| pub(super) struct DropShimVTable { |
There was a problem hiding this comment.
Why not reuse OpaqueImageVTable ?
There was a problem hiding this comment.
Maybe! I'll have a look on Monday.
3 participants
Imagebeing a different size depending on featureset has apparently caused miscompilation before, and it's somewhat of a footgun when includingImagein other types which are used in ffi. Another issue is that it bloats the size ofImagewhether or not the variants are being used.Imageis stored unboxed all over the codebase, and particularly in generated code.Size comparison
A struct or component that has many image fields is wasting up to ~70% of its space storing what is essentially just padding. Consider that Slint does not have a variant/union type, so even in cases where fields are mutually exclusive they cannot reuse space. This means that it's not uncommon in larger applications for structs/components to have many fields sequentially. A downside is that embedded images now have an extra pointer of indirection. From looking at the data actually stored by
EmbeddedImageit seems like this is possible to solve, but doing so would require significantly complicating the type and so it's beyond the scope of this PR.In the future, we may be able to take advantage of alignment niches, which, depending on the alignment of the types in
ImageInner, may bring the size all the way down to 16 bytes. Without those "niche" optimisations it's not possible to reduce the size of the type any further without changing howVRcworks.Size on different platforms is not unified, so for example Wasm and native compilation may still result in different sizes of
Image(I haven't checked). A static assertion is added, to ensure thatImageInneris the same size as the version of the enum with all features disabled.My approach involved slightly changing some constructors in the C++ Image bindings, so I'd appreciate if someone could take a look at that side of things